Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(elasticloadbalancingv2): Add support for application cookies #13142

Merged
merged 23 commits into from
Feb 24, 2021
Merged

feat(elasticloadbalancingv2): Add support for application cookies #13142

merged 23 commits into from
Feb 24, 2021

Conversation

robertd
Copy link
Contributor

@robertd robertd commented Feb 19, 2021 •
edited
Loading

https://aws.amazon.com/about-aws/whats-new/2021/02/application-load-balancer-supports-application-cookie-stickiness/

Maybe I shouldn't have deprecated stickinessCookieDuration ... which is essentially a rename to loadBalancerStickinessCookieDuration.... but that can be easily reverted based on the feedback I recieve.

As always... feedback is much appreciated. Thanks! 👍

edit:

lb_cookie

Screen Shot 2021-02-18 at 5 14 31 PM

app_cookie
Screen Shot 2021-02-18 at 5 14 16 PM

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Feb 19, 2021 •
edited
Loading

@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 label Feb 19, 2021
@robertd
Copy link
Contributor Author

robertd commented Feb 19, 2021

The more I think about this... the more I want to simply reuse stickinessCookieDuration and update all the docs. Pretty much if you provide stickinessCookieDuration param then it's lb_cookie, and if you provide both stickinessCookieDuration & appCookieName params then it becomes app_cookie. Not sure if that would confuse the users though :).

@robertd
Copy link
Contributor Author

robertd commented Feb 23, 2021 •
edited
Loading

The more I think about this... the more I want to simply reuse stickinessCookieDuration and update all the docs. Pretty much if you provide stickinessCookieDuration param then it's lb_cookie, and if you provide both stickinessCookieDuration & appCookieName params then it becomes app_cookie. Not sure if that would confuse the users though :).

@njlynch When time permits let me know what your initial thoughts are and I'll definitely modify this PR based on your guidance/feedback. Thanks!

@njlynch
Copy link
Contributor

njlynch commented Feb 23, 2021

I agree -- I think we can keep the stickinessCookieDuration as working for both, and the user can set the appCookieName if they want to base stickiness based on the application cookie. Given you can't set both (LB- and app-based stickiness), I think overloading the cookie duration won't be needlessly confusing.

@robertd
Copy link
Contributor Author

robertd commented Feb 23, 2021

Thanks @njlynch... I'm on it.

@robertd
Copy link
Contributor Author

robertd commented Feb 24, 2021

@njlynch fixed... please review.

njlynch
njlynch previously requested changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Lots of minor nit-picks.

packages/@aws-cdk/aws-elasticloadbalancingv2/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts Show resolved Hide resolved
packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/target-group.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.stickiness.ts Outdated Show resolved Hide resolved
@robertd @njlynch
Update packages/@aws-cdk/aws-elasticloadbalancingv2/README.md
7442bed 
Co-authored-by: Nick Lynch <nlynch@amazon.com>
@mergify mergify bot dismissed njlynch’s stale review February 24, 2021 15:53

Pull request has been modified.

robertd and others added 4 commits February 24, 2021 08:53
@robertd @njlynch
Update packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/applicati…
f922c6b 
…on-listener.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
@robertd @njlynch
Update packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/applicati…
ed8c0ca 
…on-listener.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
@robertd @njlynch
Update packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/applicati…
0d1ea05 
…on-target-group.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
@robertd @njlynch
Update packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/target-g…
ca19300 
…roup.test.ts

Co-authored-by: Nick Lynch <nlynch@amazon.com>
This was referenced Mar 8, 2021
Closed
Merged
Merged
Merged
Closed
Closed
Closed
Closed
Merged
Closed
Merged
Merged
Closed
Closed
Closed
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Merged
Closed
Closed
Closed
@njlynch njlynch mentioned this pull request Jun 22, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njlynch njlynch njlynch approved these changes

Assignees

@njlynch njlynch

Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@robertd @njlynch @aws-cdk-automation